Skip to content

Update warning message for PR reviewers - #761

Open
AliSoftware wants to merge 2 commits into
trunkfrom
danger-reviewers-note
Open

Update warning message for PR reviewers#761
AliSoftware wants to merge 2 commits into
trunkfrom
danger-reviewers-note

Conversation

@AliSoftware

@AliSoftware AliSoftware commented Jul 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Now that we have the apps-infra-tooling subteam (targeting a more focused subset of our team), better suggest to add that one (which should already be automatically added via the CODEOWNERS on this repo in most PRs anyway) rather than suggesting the wider apps-infrastructure team

What does it do?

Checklist before requesting a review

  • Run bundle exec rubocop to test for code style violations and recommendations.
  • Add Unit Tests (aka specs/*_spec.rb) if applicable.
  • Run bundle exec rspec to run the whole test suite and ensure all your tests pass.
  • Make sure you added an entry in the CHANGELOG.md file to describe your changes under the appropriate existing ### subsection of the existing ## Trunk section.
  • If applicable, add an entry in the MIGRATION.md file to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider.

@AliSoftware
Update warning message for PR reviewers
bad62b1 
Now that we have the `apps-infra-tooling` subteam (targeting a more focused subset of our team), better suggest to add that one (which should be automatically added via the CODEOWNER-like GitHub Settings on this repo anyway) rather than suggesting the wider `apps-infrastructure` team
@AliSoftware
AliSoftware requested a review from a team as a code owner July 28, 2026 21:39
@dangermattic

dangermattic commented Jul 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator
1 Warning
⚠️ No reviewers have been set for this PR yet. Please request a review from @⁠wordpress-mobile/apps-infra-tooling.

Generated by 🚫 Danger

@AliSoftware

AliSoftware commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@iangmaia It seems that Dangermattic's github_utils.requested_reviewers? does not detect when the only reviewers that have been requested are a GitHub Team? (like apps-infra-tooling)? Or maybe it is because that reviewer has been auto-added via CODEOWNERS?

Anyway, seems like a bug needing addressing in Dangermattic, ideally before we merge that PR

@oguzkocer oguzkocer mentioned this pull request Jul 28, 2026
Merged
oguzkocer
oguzkocer reviewed Jul 28, 2026
View reviewed changes

@oguzkocer oguzkocer left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this entirely addresses the issue. The confusing part for me was that Danger said no reviewers were assigned even though wordpress-mobile/apps-infra-tooling was already assigned due to being code owners.

One thing I noticed was that I don't always see the code owner reviewer assignment immediately after creating the PR. So, I wonder if the unless github_utils.requested_reviewers? check is happening during that brief time.

Also, I can't help but wonder, does this repo even need the warning if every PR is automatically getting assigned reviewers?

Edit: I saw this comment after I wrote all this 😞

@mokagio mokagio mentioned this pull request Jul 29, 2026
Open
@mokagio

mokagio commented Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Gio here: I landed on this PR and told Claude Code "work on Olivier's comment". This is the result. I haven't verified it myself yet. I'll update this comment once I do. Wanted to add the context in the meantime in case I get sidetracked.

@AliSoftware @oguzkocer — it is a real Dangermattic bug, and it's the team part rather than the CODEOWNERS race.

github_utils.requested_reviewers? read requested_teams off pr_json. GitHub strips that field from the pull request payload for tokens that can't see the organization's teams, and dangermattic is an outside collaborator on wordpress-mobile, not a member — so it always saw [].

Checked against this PR, which still has a pending apps-infra-tooling request:

[org-visible token] pr_json['requested_teams'] -> ["apps-infra-tooling"]
[org-visible token] review_requests['teams']   -> ["apps-infra-tooling"]

[no org visibility] pr_json['requested_teams'] -> []
[no org visibility] review_requests['teams']   -> ["apps-infra-tooling"]

GET /pulls/{n}/requested_reviewers reports teams either way, so Automattic/dangermattic#141 switches to it.

Two things that muddied the diagnosis:

  • Not a timing race. The team was requested at 21:39:22 and the Danger run that produced the warning ran 21:42:17–21:43:00.
  • The bug has been latent here since 2024. requested_reviewers? falls back to active_reviewers?, and Copilot's automatic review usually lands first — so it only shows up on a PR Copilot hasn't reviewed yet, like this one.

Nothing to change on this PR beyond waiting for a Dangermattic release; the message update stands on its own.

Posted by Claude Code (Opus 5) on behalf of @mokagio with approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oguzkocer oguzkocer oguzkocer left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AliSoftware @dangermattic @mokagio @oguzkocer